Skip to content

OIDC logout support (PP-3726)#3120

Merged
tdilauro merged 27 commits intomainfrom
feature/saml-oidc-logout
Mar 22, 2026
Merged

OIDC logout support (PP-3726)#3120
tdilauro merged 27 commits intomainfrom
feature/saml-oidc-logout

Conversation

@tdilauro
Copy link
Contributor

@tdilauro tdilauro commented Mar 9, 2026

Description

Improves the initial OIDC RP-Initiated Logout flow with several enhancements:

  • Bearer-token-based logout: The logout endpoint now reads token data directly from the CM Authorization Bearer header rather than requiring the client to pass an id_token_hint query parameter. This avoids a DB lookup that would fail after token refresh (since the credential value changes on refresh but the patron's bearer token still holds the old value).
  • Stored id_token for logout hint: The raw ID token JWT is now persisted in the credential JSON at authentication (and on refresh), so it can be used as id_token_hint in RP-Initiated Logout requests.
  • Token revocation support (RFC 7009): Added a new optional revocation_endpoint configuration field. On logout, the OAuth refresh token is revoked best-effort to prevent silent re-authentication at the IdP after local CM session invalidation.
  • Graceful fallback: If the provider does not support RP-Initiated Logout, or if no stored id_token is available (credentials predating this feature), logout completes locally and redirects to success without an IdP round-trip.
  • Logout link in authentication document: If the provider supports any logout mechanism, a "rel": "logout" link is now included in the authentication document returned to clients.
  • Authentication manager caching: get_authentication_manager() now caches the OIDCAuthenticationManager instance for the provider's lifetime, preventing repeated OIDC discovery document fetches on every authenticated request.

Motivation and Context

  • Fixes and extends the OIDC logout flow introduce with the initial OIDC implementation. The original implementation required clients to pass an id_token_hint, which was fragile after token refresh (the stored credential changes but the patron's bearer token does not). This rework makes logout more reliable and self-contained, using only the bearer token already required for authenticated requests.

  • Also addresses a performance issue where the OIDC discovery document was being fetched on every request to get_authentication_manager().

[Jira PP-3726]

How Has This Been Tested?

  • Test pass locally.
  • CI tests passed.

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@tdilauro tdilauro requested a review from a team March 9, 2026 15:22
@codecov
Copy link

codecov bot commented Mar 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.28%. Comparing base (6616698) to head (d4c0f63).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3120      +/-   ##
==========================================
+ Coverage   93.26%   93.28%   +0.01%     
==========================================
  Files         493      493              
  Lines       45615    45711      +96     
  Branches     6254     6264      +10     
==========================================
+ Hits        42545    42643      +98     
+ Misses       1983     1982       -1     
+ Partials     1087     1086       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbernstein
Copy link
Contributor

Other minor issues:

WIP commit message
One commit message still says “WIP - experimenting, but problems with endpoint logout”. Consider squashing or rewording before merge.

OIDC_INVALID_ID_TOKEN_HINT removal
Removing OIDC_INVALID_ID_TOKEN_HINT is consistent with no longer requiring id_token_hint. Confirm there are no remaining references (e.g. in tests or docs).

Test coverage for post_logout_redirect_uri validation
If validation of post_logout_redirect_uri is added later, tests should cover both valid and invalid URIs.

Documentation for revocation_endpoint
The new revocation_endpoint configuration is documented in the model. A short note in admin or deployment docs would help operators.

Copy link
Contributor

@dbernstein dbernstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tdilauro : this looks great overall. I'm giving it the thumbs up. There are some minor comments to consider.

There is one issue that I believe should be addressed before merging: ie validating the redirect url. But I leave it to you to make the call.

@tdilauro tdilauro force-pushed the feature/saml-oidc-logout branch from 13df2ba to 4526235 Compare March 18, 2026 01:51
@tdilauro tdilauro force-pushed the feature/saml-oidc-logout branch from 96d13a5 to d4c0f63 Compare March 22, 2026 00:24
@tdilauro tdilauro merged commit a5d2c7c into main Mar 22, 2026
20 checks passed
@tdilauro tdilauro deleted the feature/saml-oidc-logout branch March 22, 2026 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants